Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libsubprocess: increase output watcher priority #6317

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Sep 26, 2024

Problem: If a output buffer is full, we issue an emergency call to the user's output callback to empty the buffer before more data is put in it. This is done because we cannot control the order in which check callbacks are called. Now that check callbacks can have priority set, we can ensure output check callbacks are called first before a check callback that may put more data in the buffer.

Set the priority of the output check watcher higher than the default. Remove the now unnecessary emergency callback to the user's output callback.

built on top of #6316

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@chu11 chu11 force-pushed the issue6302_priority_watcher branch from f124c7e to db04615 Compare September 27, 2024 06:49
@grondo
Copy link
Contributor

grondo commented Oct 1, 2024

Let's add MWP here so it makes it into today's release

@chu11 chu11 force-pushed the issue6302_priority_watcher branch from db04615 to 7523530 Compare October 1, 2024 23:43
Problem: If a output buffer is full, we issue an emergency call
to the user's output callback to empty the buffer before more
data is put in it.  This is done because we cannot control the
order in which check callbacks are called.  Now that check
callbacks can have priority set, we can ensure output check
callbacks are called first before a check callback that may put
more data in the buffer.

Set the priority of the output check watcher higher than the
default.  Remove the now unnecessary emergency callback to the
user's output callback.

Fixes flux-framework#6302
@chu11 chu11 force-pushed the issue6302_priority_watcher branch from 7523530 to a7f99af Compare October 1, 2024 23:56
@mergify mergify bot merged commit 9111eed into flux-framework:master Oct 2, 2024
33 checks passed
@chu11 chu11 deleted the issue6302_priority_watcher branch October 2, 2024 17:57
Copy link

codecov bot commented Dec 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.33%. Comparing base (51fed58) to head (a7f99af).
Report is 475 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6317      +/-   ##
==========================================
- Coverage   83.33%   83.33%   -0.01%     
==========================================
  Files         523      523              
  Lines       86167    86166       -1     
==========================================
- Hits        71808    71804       -4     
- Misses      14359    14362       +3     
Files with missing lines Coverage Δ
src/common/libsubprocess/remote.c 78.34% <100.00%> (-0.09%) ⬇️

... and 8 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants